Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add OpenTelemetry feature into Gravitee Node #374

Draft
wants to merge 1 commit into
base: alpha
Choose a base branch
from

Conversation

guillaumelamirand
Copy link
Contributor

@guillaumelamirand guillaumelamirand commented Oct 11, 2024

Issue

https://gravitee.atlassian.net/browse/ARCHI-401

Description

The objective is to provide a generic way for Gravitee Application to manage tracing based on OpenTelemetry specification.

Additional context


Gravitee.io Automatic Deployment

🚀 A prerelease version of this package has been published on Gravitee's private artifactory, you can:

  • use it directly by updating your project with version: 7.0.0-archi-401-opentelemetry-SNAPSHOT
  • download it from Artifactory here

@guillaumelamirand guillaumelamirand force-pushed the archi-401-opentelemetry branch 4 times, most recently from 26c1990 to 53eee18 Compare October 16, 2024 15:37
@guillaumelamirand guillaumelamirand force-pushed the archi-401-opentelemetry branch 9 times, most recently from 7f12533 to 6b45634 Compare October 24, 2024 15:39
BREAKING CHANGE: Tracing plugin has been removed and is now embedded inside node framework
this.environment = environment;
}

@Value("${services.opentelemetry.enabled:${services.tracing.otel.traces.enabled:true}}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that the services.opentelemetry.enabled is set to false by default in the gravitee.yaml of APIM which avoids to fallback to the services.tracing.otel.traces.enabled value.

Also, I can't remember where services.tracing.otel.traces.enabled comes from. Can you confirm (or not) that it isn't services.tracing.enabled instead?

@@ -0,0 +1,26 @@
/*
* Copyright (C) 2015 The Gravitee team (http://gravitee.io)

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

 http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the license header is broken

@@ -0,0 +1,15 @@
/*
* Copyright (C) 2015 The Gravitee team (http://gravitee.io)

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

 http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

* @author Guillaume LAMIRAND (guillaume.lamirand at graviteesource.com)
* @author GraviteeSource Team
*/
public record ObservableHttpServerRequest(HttpServerRequest httpServerRequest) implements ObservableHttpRequest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it should not be moved into the node-vertx instead (assuming it doesn't cause any trouble)?
Same question for other records.

<gravitee-kubernetes.version>3.1.0</gravitee-kubernetes.version>
<snakeyaml.version>2.0</snakeyaml.version>
<hazelcast.version>5.5.0</hazelcast.version>
<gravitee-alert-api.version>1.9.1</gravitee-alert-api.version>
<opentelemetry.version>1.39.0</opentelemetry.version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a most recent version (1.43.0)

}

private SpanExporter createGrpcSpanExporter(final URI tracesUri) {
return new VertxGrpcSpanExporter(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember you presented this part during a live session but I'm afraid I forgot the reasons behind the implementation of the VertxGrpcSpanExporter.
I did a quick test using OtlpGrpcSpanExporter provided by opentelemetry and it does the job so I'm wondering if we should give it a try (it would save us from a lot of copy-pasted code that are kind of complex and will probably be hard to maintain).

For the record, here is how I quickly tested it:

    private SpanExporter createGrpcSpanExporter(final URI tracesUri) {
        return OtlpGrpcSpanExporter
                .builder()
                .setEndpoint(openTelemetryConfiguration.getEndpoint())
                .setCompression(openTelemetryConfiguration.getCompressionType().value())
                .setTimeout(openTelemetryConfiguration.getTimeout())
                .setHeaders(this::populateTracingExportHttpHeaders)
                .setMeterProvider(MeterProvider::noop)
                .build();
    }

@@ -0,0 +1,23 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Header broken


SdkTracerProvider tracerProvider = SdkTracerProvider
.builder()
.addSpanProcessor(BatchSpanProcessor.builder(exporterFactory.createSpanExporter()).build())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the span exporter establishes connections and deals with batching, I'm wondering if we could find a way to share it instead of creating one for each tracer. This could save resources (connections, workers, ...). It comes with an extra effort to manage its lifecycle which, according to the javadoc, it is tight to the tracer's lifecycle but worth the pain. WDYT?

@@ -0,0 +1,129 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it is a new node module, can you add a readme similar to what has been done for cluster or cache 🙏 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants